Tagging subscribers to this area: @dotnet/area-system-text-regularexpressions
See info in area-owners.md if you want to be subscribed.

/azp run runtime-coreclr crossgen2 outerloop

cc: @tannergooding, @trylek

On second thought, while it's great to see that the Crossgen2 legs are passing, it actually doesn't say much about Crossgen2 and static interface functionality w.r.t. your change as I believe we still don't have library tests that would run Crossgen2 on the test IL code - we only do that for the framework assemblies. Still, I presume that if the relevant tests pass locally for you with Crossgen2 enabled, it should be fine. Some time ago @danmoseley offered help in finding someone to look into adding infra support for crossgenning library tests as I myself am not really an expert on library test build & execution infra but I'm not aware of any follow-up so far.

Still, I presume that if the relevant tests pass locally for you with Crossgen2 enabled, it should be fine.

Can you elaborate? What would I do locally that CI doesn't do?

Well, maybe it's not a concern at all. The one thing we're basically unable to test today is running a Crossgen2-compiled library test. Considering you're only changing bits within the framework that does get Crossgen2-compiled by itself, it's probably not a big deal, however it's still a test gap especially considering that Tanner recently raised the concern that static virtual methods may not fully function in Crossgen2 context.

Well, maybe it's not a concern at all

Thanks. Even if it's not causing failures for crossgen, it sounds like it would still cause more of the assembly to fall back to JIT? Do we consider that a problem?

Also, @lambdageek, this passed all mono legs, but presumably this still shouldn't be merged until mono's fixes are in? Is there some leg I should run to validate that?

Some time ago @danmoseley offered help in finding someone to look into adding infra support for crossgenning library tests as I myself am not really an expert on library test build & execution infra but I'm not aware of any follow-up so far.

@agocke @ericstj is this on our infrastructure backlog somewhere (where it's relative priority lies I do not know)?

@danmoseley it looks like this issue: #50127

It's not currently high on my backlog. It may help crossgen2 find bugs, but crossgen'ing the framework is already fairly expensive. I'm worried it could significantly degrade build perf.

Could we enable it as an outerloop/on-demand job?

I guess that running such a job once or twice a week or on demand should be sufficient. To be precise, I believe there are two aspects to the issue:

One is using CG2-compiled framework - we're already doing that for CoreCLR CG2 testing (as part of generating CORE_ROOT) but I think that library tests use a different method to access the framework assemblies and that today doesn't involve CG2 compilation at all. (CG2 compilation does run on the framework assemblies during "packs", previously "installer", builds, but AFAIK the product of this compilation is only used to run the "packs"-specific tests, not library tests, in the runtime repo.)

The second aspect is CG2 compilation of the tests themselves and that is supposed to require non-trivial modifications to the library test build scripts. I just checked a random CG2 outerloop run and composite compilation of framework takes about 3 minutes, non-composite about 5~6 minutes; I wouldn't be too worried about test compilation performance as library tests use less than two hundred merged test executables i.o.w. I would expect their compilation to take even less time than CG2-compiling the framework. And after all execution of the crossgenned apps should be faster, at the very least in terms of startup perf.

In general, both aspects add code coverage of various CG2 compilation constructs including the static virtual methods. The existing code coverage in Crossgen2 pipelines amounts to validating that the compiler doesn't crash when presented with a new construct in the framework (like in this change) and that the CoreCLR tests run fine on top of the crossgenned framework but I suspect that targeted testing of SymbolicRegexMatcher in CoreCLR tests is very limited, certainly more limited than testing of the feature in the library test set.

Adding it as part of an outerloop sounds fine to me, but I would expect that would be led primarily by the product team w/ assistance from infra where needed.

Well, I can easily deal with the ceremony regarding proper setup of the YAML scripts and such, my only problem is fixing the library test build scripts themselves to support Crossgen2 compilation - I have never made changes in that space before; the last time we discussed this Viktor Hofer also suggested that support for Crossgen2 compilation may entail some generally desirable long-planned cleanup of the scripts; I must admit that due to my limited knowledge of these scripts I don't know what precisely he referred to, perhaps @safern or @jkoritzinsky may have a better idea.

/azp run runtime-manual

Also, @lambdageek, this passed all mono legs, but presumably this still shouldn't be merged until mono's fixes are in? Is there some leg I should run to validate that?

@steveisok @akoeplinger Do we have any legs that run libraries tests with FullAOT that @stephentoub should pay attention to?

To put it yet another way, I'll be happy to work with someone familiar with library test build scripts to get this fixed by providing Crossgen2-related expertise but I feel reluctant to drill into this just by myself as I simply won't be efficient in that space. I think it basically involves the following changes:

(*) Identifying the place where the framework gets passed to Helix, probably as a correlation payload, and modifying that place to support optional CG2 compilation, probably by using the existing logic in the SDK repo.

(*) Deciding whether CG2 compilation of the test assemblies should happen on the build machine or on the Helix execution machines. In the former case it should become a part of producing the test payloads for Helix, in the latter case it should be part of some execution-time scripting running on the Helix machines and we need to figure out how to make Crossgen2 built for the proper OS and architecture available to Helix, possibly as yet another correlation payload. (In CoreCLR testing it is passed via CORE_ROOT.) Crossgen2 is a managed app so we also need the .NET Host on the Helix machines.

(*) Using the above components, typically controlled by some environment variables or by command-line arguments to the build scripts, to build the new pipeline. I can easily deal with that part once the library script changes are in place.

Thanks

Tomas

@trylek I can definitely help with any questions/blockers you may find when enabling this crossgen'd mode with any related to running crossgen on the shared framework we generate to run our tests or as part of the libraries test build.

Thanks Santi for your kind offer of help. I'll mention it in today Jeff's backlog meeting and we can then schedule a separate chat about the technical details I outlined.

/azp list

Sorry, it's runtime-extra-platforms

/azp run runtime-extra-platforms

I assume the failures like these are the indication of the lack of static abstract interface support (it would require falling back to JIT but falling back to JIT isn't allowed):

System.ExecutionEngineException : Attempting to JIT compile method 'System.Text.RegularExpressions.Symbolic.DfaMatchingState1 System.Text.RegularExpressions.Symbolic.SymbolicRegexMatcher1:Delta<System.Text.RegularExpressions.Symbolic.SymbolicRegexMatcher1/BrzozowskiTransition> (System.ReadOnlySpan1,int,System.Text.RegularExpressions.Symbolic.DfaMatchingState`1)' while running in aot-only mode. See https://docs.microsoft.com/xamarin/ios/internals/limitations for more information.

I assume the failures like these are the indication of the lack of static abstract interface support (it would require falling back to JIT but falling back to JIT isn't allowed):

System.ExecutionEngineException : Attempting to JIT compile method 'System.Text.RegularExpressions.Symbolic.DfaMatchingState1 System.Text.RegularExpressions.Symbolic.SymbolicRegexMatcher1:Delta<System.Text.RegularExpressions.Symbolic.SymbolicRegexMatcher1/BrzozowskiTransition> (System.ReadOnlySpan1,int,System.Text.RegularExpressions.Symbolic.DfaMatchingState`1)' while running in aot-only mode. See https://docs.microsoft.com/xamarin/ios/internals/limitations for more information.

Yea it looks like we are missed AOTing an instance here. @vargaz Delta<T>() is a generic method, and T gets instantiated with BrzozowskiTransition which is a struct. the body of Delta calls a static virtual method on T. Do we bail out of AOTing this method for some reason? (this is fullaot for tvOS)

Made a new issue for the the mono FullAOT fail #65002

Thanks, @lambdageek.

@stephentoub merge/rebase with main to pick up #65126 which should address the tvOS failures

/azp run runtime-extra-platforms

At this point inner loop passed, there are no regex-related failures in runtime-extra-platforms, and the Crossgen2 legs previously all passed. Any remaining static abstract interface method reasons this can't be merged?
cc: @tannergooding

Based on the fact that the known blockers are no longer blockers, I'm going to merge. Long live static abstract interface methods.